fix stream-file urls to use new remoteuri#1984
Conversation
WalkthroughThe pull request introduces changes across both frontend and backend components. On the frontend, modifications to URL handling in Electron event handlers and markdown utilities adjust how remote file paths are constructed, incorporating file basenames in URLs and using a new URI formatting function. Conditional rendering logic in UI components is refactored by introducing new variables and state atoms, including adjustments for S3 suggestion visibility. Additionally, a new utility function for formatting remote URIs has been added, replacing previous methods of building file URLs. Changes on the backend in the web module add functions for handling local file streaming and modify the logic for processing remote file streams, updating the overall file streaming mechanism. New functions have been introduced, and existing functions have been updated to improve the clarity and functionality of the application. No alterations were made to the exported or public entity signatures, aside from the addition of new variables and properties controlling state. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/app/element/markdown-util.ts (1)
162-168:Details
❓ Verification inconclusive
Verify baseDir usage & consider explicit error handling
Currently, the baseDir is converted to a remote URI usingformatRemoteUri. IfresolveOpts.baseDiris empty or malformed, the final path might be unexpected. Consider validatingbaseDirand adding explicit error handling or fallback logic to reduce potential edge cases (e.g., empty baseDir, unusual characters).You can run the following script to scan for invocation points of
resolveRemoteFileto ensure thatresolveOpts.baseDiris always valid:
🏁 Script executed:
#!/bin/bash # Find all usage sites of resolveRemoteFile to confirm correct usage of baseDir rg -A 4 'resolveRemoteFile\('Length of output: 754
Action Required: Validate
baseDirand Implement Explicit Error HandlingIn
frontend/app/element/markdown-util.ts(lines 162–168), the code currently convertsresolveOpts.baseDirto a remote URI without performing any explicit checks. This may lead to issues ifresolveOpts.baseDiris empty or malformed.
- Please add explicit validation for
resolveOpts.baseDirbefore using it withformatRemoteUri.- Consider implementing error handling or fallback logic for edge cases (e.g., empty strings or unexpected characters) to ensure that the joined path and subsequent remote URI are reliable.
- Note that similar usage patterns are observed in both
markdown-util.tsandmarkdown.tsx(viaresolveRemoteFile), suggesting that inconsistent handling ofbaseDirmight propagate unexpected results elsewhere in the app.frontend/util/waveutil.ts (1)
10-12: Ensure path safety when creating stream-file URLs
UsingformatRemoteUri(file, "local")is consistent, but remains vulnerable iffilecontains special characters or malicious input. Consider sanitizing or validating the path before encoding and appending.frontend/app/view/term/termsticker.tsx (1)
122-123: Unify local streaming approach with waveutil
Switching to/wave/stream-local-fileis consistent with your new endpoint. You might consider leveraging a shared helper (e.g.,formatRemoteUri) to uniformly build local vs. remote streaming URLs and reduce duplication across the codebase.pkg/web/web.go (1)
331-353: Consider improving error message specificity.While the implementation is solid, the error message could be more specific about what went wrong during streaming.
Consider this improvement:
- http.Error(w, fmt.Sprintf("error streaming file: %v", err), http.StatusInternalServerError) + http.Error(w, fmt.Sprintf("failed to stream file %q: %v", path, err), http.StatusInternalServerError)emain/emain.ts (1)
242-245: LGTM! URL construction improved with basename in path.The changes enhance the URL structure by including the file basename in the path segment, making it more identifiable and SEO-friendly. The use of
encodeURIComponentfor both basename and full path ensures proper URL encoding.Consider using template literals for better readability:
- const streamingUrl = - getWebServerEndpoint() + "/wave/stream-file/" + baseName + "?path=" + encodeURIComponent(payload.filePath); + const streamingUrl = `${getWebServerEndpoint()}/wave/stream-file/${baseName}?path=${encodeURIComponent(payload.filePath)}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
emain/emain-util.ts(1 hunks)emain/emain.ts(1 hunks)frontend/app/block/blockframe.tsx(2 hunks)frontend/app/element/markdown-util.ts(2 hunks)frontend/app/modals/conntypeahead.tsx(2 hunks)frontend/app/view/preview/directorypreview.tsx(2 hunks)frontend/app/view/preview/preview.tsx(4 hunks)frontend/app/view/term/termsticker.tsx(1 hunks)frontend/types/custom.d.ts(1 hunks)frontend/util/waveutil.ts(2 hunks)pkg/web/web.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (13)
frontend/app/element/markdown-util.ts (1)
7-7: Import aligns with new URI formatting approach
The import correctly referencesformatRemoteUrifrom waveutil, ensuring consistent URI handling across the application.emain/emain-util.ts (1)
89-94: LGTM! URL validation expanded correctly.The additional URL patterns for streaming endpoints are properly validated, maintaining security while supporting both trailing slash and local file streaming URLs.
pkg/web/web.go (3)
249-256: LGTM! Improved stream handling with proper cleanup.The function now correctly delegates to
handleRemoteStreamFileFromChand includes stream cancellation functionality.
258-319: LGTM! Well-implemented streaming handler with proper resource management.The function includes:
- Proper context cancellation handling
- Resource cleanup
- Appropriate header management
- Graceful error handling
321-329: LGTM! Proper parameter validation and error handling.The function correctly validates the path parameter and handles local file streaming requests.
frontend/types/custom.d.ts (1)
295-297: LGTM! Well-documented and properly typed property.The
showS3property is correctly implemented as an optional boolean atom with clear documentation.frontend/app/modals/conntypeahead.tsx (1)
351-351: LGTM! Proper integration of S3 visibility control.The changes correctly implement conditional S3 suggestion rendering based on the new
showS3atom.Also applies to: 440-450
frontend/app/block/blockframe.tsx (1)
244-244: LGTM! Improved button visibility control with clear variable name.The introduction of
showNoWshButtonenhances code readability by clearly expressing when the wsh installation button should be shown. The conditions are well-defined and handle edge cases like blank connection names and AWS connections.Also applies to: 267-267
frontend/app/view/preview/directorypreview.tsx (1)
579-580: LGTM! Improved URI handling with centralized formatting.The changes enhance consistency by using the centralized
formatRemoteUrifunction for constructing remote URIs during file downloads.frontend/app/view/preview/preview.tsx (4)
184-184: LGTM! Added state management for S3 visibility.The introduction of
showS3atom with a default value oftrueprovides clear state management for S3-related UI elements.
942-945: LGTM! Improved URI handling in StreamingPreview.The changes enhance consistency by using the centralized
formatRemoteUrifunction and properly constructing the URL parameters.
949-949: Use strict equality for MIME type comparison.The change from loose equality (
==) to strict equality (===) for MIME type comparison is a good practice.
1311-1311: LGTM! Simplified exports.The changes clean up the exports by removing the
formatRemoteUrifunction and only exporting thePreviewViewcomponent.
| export function formatRemoteUri(path: string, connection: string): string { | ||
| connection = connection ?? "local"; | ||
| // TODO: We need a better way to handle s3 paths | ||
| let retVal: string; | ||
| if (connection.startsWith("aws:")) { | ||
| retVal = `${connection}:s3://${path ?? ""}`; | ||
| } else { | ||
| retVal = `wsh://${connection}/${path}`; | ||
| } | ||
| return retVal; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add path sanitization for remote URI construction
The formatRemoteUri function delegates to custom schemes (aws: vs wsh://), but does not currently sanitize or validate path. Unchecked input might cause incorrect or unintended URIs. Consider adding path checks (removing illegal characters, validating emptiness, etc.) to reinforce correctness and security.
1 participant
No description provided.